-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bug that coerced empty scaled float value to 0 #62251
Fix bug that coerced empty scaled float value to 0 #62251
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM. I left a comment around the actual default value we use.
@@ -502,7 +502,7 @@ export const PARAMETERS_DEFINITION: { [key in ParameterName]: ParameterDefinitio | |||
} | |||
), | |||
fieldConfig: { | |||
defaultValue: '', | |||
defaultValue: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should check with an ES engineer to see what a good default value might be; 1
seems a little weird to me, since it's used as a multiplier. The documentation uses 10
as an example.
Docs: https://www.elastic.co/guide/en/elasticsearch/reference/current/number.html#scaled-float-params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave it as 1, as multiplying by one does not do anything. The default value should be what ES uses behind the scene in case the parameter is not specified, and I think by default ES does not scale float.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seb's point is also what I originally had in mind (1 is a good default because it's basically a no-op). But Colin pointed out that it would be a better UX to force the user to consider the option and enter the right value for their use case. I think he's right. It's not helpful to make it easy for users to use a scaled float that doesn't take advantage of the scaling factor (a different type would be better).
Unless anyone objects, I intend to:
- Close this PR
- Update the original issue with these notes
- Update the original issue with a note that we should also validate against a value of
1
and warn the user that this isn't very useful - Create a blocker issue that we need to update the form hook lib to allow numeric fields to be empty instead of defaulting to 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 sounds good. I agree that leaving it blank would be the better experience. Thanks @cjcenizal!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked, but my guess is that it is the EuiNumeric field that defaults to 0.
I would see the reason being: an empty string (""
) is not a valid number, thus we render 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the conversion from ""
to 0
occurs on the field deSerializer
that we have set
deserializer: (value: string | number) => +value,
// Probably needs to be
deserializer: (value: string | number) => value === '' ? '' : +value,
… coerced this to 0.
@elasticmachine merge upstream |
Good catch @sebelga! I've fixed this. Could you take another look @alisonelizabeth? Now the user is required to enter input: @sebelga When you're writing the docs for the form hook lib, you might want to consider that people could find the roles of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works great now. Thanks @cjcenizal!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…into event-log/query-support * 'event-log/query-support' of github.com:gmmorris/kibana: (41 commits) [jenkins] refer to sizes in most pipeline code (elastic#62082) skip flaky suite (elastic#60470) [Discover] Fix flaky FT in field visualize (elastic#62418) [ML] Data Frame Analytics: Fix feature importance (elastic#61761) [Reporting] Use a shim for server config (elastic#62086) [Reporting] Fix reporting for non-default spaces (elastic#62226) Fix bug that coerced empty scaled float value to 0 (elastic#62251) [SIEM] [Detection Engine] Remove has manage api keys requireme… (elastic#62446) [Maps] Safely handle empty string and invalid strings from EuiColorPicker (elastic#62507) Reporting/bug more blacklisted headers (elastic#62389) [SIEM] Prevent undefined behavior in our ML popover (elastic#62498) [SIEM] [Detection Engine] remove all unknowns from all rules t… (elastic#62327) base changes for active/current node styling (elastic#62007) [kbn/ui-shared-deps] expand and split (elastic#62364) [ML] DF Analytics - ensure destination index pattern created (elastic#62450) Mark rule run as failure if there was an error (elastic#62383) Add docs for metric explorer alerts (elastic#62314) skip flaky suite (elastic#62281) [SIEM][Detection Engine] Fixes export of single rule and the icons fixes flakiness (elastic#62406) ...
Thanks for the insight @cjcenizal, I will take it into account to clearly explain: 3 lifecycles of field value
|
Fixes #62035